Add paired browsing interface #3656

pierlon · 2019-10-28T21:48:41Z

Summary

Todo

Extract the template for paired browsing into its own file
~~Add support for paired browsing while in Standard mode.~~
Add paired browsing button to AMP Validated URL status metabox.
Provide means of exiting paired browsing mode, perhaps meaning an additional paired browsing bar appearing at top of page.
Allow paired browsing while admin bar is not showing. This will likely mean persisting certain paired-mode query vars on links and forms so that the required JS in the window will be loaded. Or else, the parent window can instead be polling for access to the iframe window contents.
If AMP window initializes without AMP query var, then show AMP not available message, and make iframe hidden. Make sure that that if AMP is disabled for a given post or template that the failure to show AMP version is handled.
Add a label to each iframe for clear identification

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

pierlon · 2019-10-28T21:58:25Z

For pages with validation errors, once the URL includes the amp_validation_errors query var it prevents any further redirection and alerts the user with the reason why the AMP version can't be viewed:

Any suggestions on how this could be improved?

schlessera · 2019-10-28T23:02:08Z

I suggest changing the background colors to something less aggressive and having big centered text in the middle of each iframe saying "non-AMP" and "AMP":

assets/src/admin/paired-browsing/app.js

.eslintrc

westonruter · 2019-10-29T00:05:35Z

I've fleshed out the todos based on additional requirements that I hadn't yet added to the corresponding issue.

westonruter · 2019-10-29T00:13:40Z

Add support for paired browsing while in Standard mode.

This is worth having a conversation about. The thinking here is that if you are debugging an issue on an AMP-first site (Standard mode), you may want to compare the AMP page with a non-AMP page to see what is going on. Think of, for example, adding a new block that tries to enqueue some JS (like a Countdown). Nevertheless, allowing paired browsing may not have the desired effect. It could be that the theme is not even designed to work without AMP, so trying to load the theme without AMP could result in a broken template.

The alternative would be to expose the debug flags (#1294, being worked on in #3448), specifically the debug option to disable AMP. Come to think of it, this would essentially be a corollary to the addition of the “Preview AMP” button (#2934 via #3323): on a Standard-mode site, the Validated URL screen should provide a “View non-AMP” link which would provide that query var to disable AMP for the request. This may have a better result than trying to force paired browsing to work in Standard mode, when there's no guarantee that non-AMP version would even work. /cc @kienstra @amedina

pierlon · 2019-10-29T02:59:17Z

I suggest changing the background colors to something less aggressive and having big centered text in the middle of each iframe saying "non-AMP" and "AMP":

Thinking about this some more: are the background colors necessary? The labels should be sufficient to identify the iframes.

westonruter · 2019-10-29T03:03:09Z

The background colors are not necessary. They were used as an initial way to clearly differentiate the two.

pierlon · 2019-10-29T08:33:38Z

In 964f981, I've added a Paired Browsing navigation bar:

pierlon · 2019-10-29T08:53:10Z

Provide means of exiting paired browsing mode

I'm not sure what this means. Would it be closing the tab that was opened, or upon exiting it redirects the parent window to the URL from one of the iframes?

I think closing the tab would be the better option here, but there is one major caveat with that approach. It is possible to close the tab once its opened from the "Start paired browsing" link, but not so if the page is accessed directly from that same link

pierlon · 2019-10-29T09:02:24Z

Allow paired browsing while admin bar is not showing.

Could a reason be given for this? I can't think of any scenarios where something like this would occur.

swissspidy · 2019-10-29T09:45:12Z

assets/src/admin/paired-browsing/client.js

+
+	app.registerClientWindow( window );
+
+	document.addEventListener( 'DOMContentLoaded', () => {


Could use @wordpress/dom-ready instead as we do already everywhere else. Example:

/** * WordPress dependencies */ import domReady from '@wordpress/dom-ready'; domReady( () => { // ... } );

It's basically the same, but also checks document.readyState

assets/src/admin/paired-browsing/app.js

webpack.config.js

swissspidy · 2019-10-29T10:00:38Z

templates/amp-paired-browsing.php

+						<img src="<?php echo esc_url( amp_get_asset_url( 'images/amp-white-icon.svg' ) ); ?>" alt="">
+					</li>
+					<li>
+						<span>Paired Browsing</span>


Suggested change

<span>Paired Browsing</span>

<span><?php esc_html_e( 'Paired Browsing', 'amp' ); ?></span>

includes/class-amp-theme-support.php

swissspidy · 2019-10-29T10:10:09Z

I'm not sure what this means. Would it be closing the tab that was opened, or upon exiting it redirects the parent window to the URL from one of the iframes?

My understanding would be the latter. If my website is the only tab open and I enter paired browsing, how do I get back to my website? Closing the tab would not be ideal.

Could a reason be given for this? I can't think of any scenarios where something like this would occur.

If I disable the admin bar from being shown on the frontend (can be done in the user profile), how can I still use paired browsing?

pierlon · 2019-10-30T06:28:41Z

Paired browsing is now accessible from the Validated URL screen:

The button only appears if both AMP is enabled for that post and transitional mode is available.

pierlon · 2019-10-30T22:12:38Z

Exiting paired browsing mode is now possible, and will redirect to the current non-AMP page:

templates/amp-paired-browsing.php

assets/src/admin/paired-browsing/app.js

pierlon · 2019-10-31T17:35:23Z

If I disable the admin bar from being shown on the frontend (can be done in the user profile), how can I still use paired browsing?

Is this what you had in mind for the todo: "Allow paired browsing while admin bar is not showing", @westonruter?

assets/src/admin/paired-browsing/app.js

westonruter · 2019-11-30T04:07:34Z

That's a valid suggestion. You could go on browsing within the iframe, but upon the load of each page you will be nagged with the same 'disconnected' overlay until the paired browsing client is available again.

You know what paired browsing will eventually be able to make excellent use of? Instead of iframes it could use portals, when they become available. This would allow seamlessly exiting the paired browsing context without having to reload the page. I think this will also work when entering paired browsing as well:

Clicking the “Start paired browsing” link on in the admin bar could create a portal that contains the paired browsing interface.
The paired browsing interface can be activated.
The previous window could then be adopted as a portal inside the paired browsing window and displayed as either the AMP or non-AMP frame, depending on if the user started from an AMP or non-AMP page.
And a second portal can then be created to contain the corresponding window (non-AMP for AMP, and vice versa), and this portal could likewise be used for exiting paired browsing.

This brings something to light. A user could actually exit to either the AMP or non-AMP window when they are done with paired browsing. At the moment it forces the user to go to the non-AMP version (see https://github.com/ampproject/amp-wp/pull/3656/files#r352265423) but eventually they could be directed to either. There's no need to implement this now, but once portals become an option, this could be a nice touch.

westonruter · 2019-11-30T04:49:55Z

I noticed a problem with the centered AMP/non-AMP frame titles. When resizing the window to show a mobile viewport, the “Paired Browsing” text overlaps the non-AMP label:

westonruter · 2019-11-30T04:57:53Z

I have an idea for fixing the overlap problem and also exit link returning the user to where they came from: we can just turn both “Non-AMP” and “AMP” into links and keep them updated to be the same as the iframe src. We can then eliminate the separate Exit link altogether, and I think we could also remove the “Paired Browsing” text as well. I'm going to try it.

westonruter · 2019-11-30T05:25:17Z

There, take a look a look at these:

pierlon · 2019-11-30T05:44:11Z

That looks much neater! I love the use of the background colors in the nav bar to help differentiating the iframe labels. The use of the labels as links to exit paired browsing is also a great idea.

You know what paired browsing will eventually be able to make excellent use of? Instead of iframes it could use portals, when they become available.

Yes! The use of that API would fit perfectly here and would greatly improve the UX. I guess the current implementation of using iframes will still be needed as a fallback, no?

westonruter · 2019-11-30T05:52:19Z

That's right. If portals aren't available then we'd fall back to iframes.

…nt/3365-paired-browsing * 'develop' of github.com:ampproject/amp-wp: (53 commits) Pull the built `block-libray` package from Gutenberg SVN if it does not exists (#3847) Update dependency @babel/plugin-transform-react-jsx to v7.7.4 (#3688) Update dependency @babel/plugin-proposal-class-properties to v7… (#3687) Update dependency @babel/cli to v7.7.4 (#3685) Update dependency browserslist to v4.7.3 (#3792) Update dependency postcss to v7.0.23 (#3791) Update dependency autoprefixer to v9.7.2 (#3679) For the Gallery block, use the recommended amp-lightbox-gallery Add the -o flag to composer install for production build processes Make get_dimensions() private instead of public Remove optimize-autoloader from composer.json config, will apply this in build Also rename images to slides in Test_Carousel In Carousel, rename $images to $slides Commit Alain's suggestion to make `$images` private Update src/Component/Carousel.php Commit Alain's suggestion to make the Carousel class final Make $elements propert private again Edit DocBlocks, include correcting var tag Update the DocBlock of DOMElementList::add() Clone the DOMElementList in add(), and return the clone ...

westonruter · 2019-11-30T08:41:50Z

includes/class-amp-theme-support.php

+	/**
+	 * Add the `data-ampdevmode` attribute to any enqueued script that the paired browsing interface
+	 * depends on.
+	 *
+	 * @since 1.3
+	 *
+	 * @param string $tag    The link tag for the enqueued script.
+	 * @param string $handle The script's registered handle.
+	 * @return string Tag.
+	 */
+	public static function filter_paired_browsing_client_script_loader_tag( $tag, $handle ) {
+		if ( self::has_dependency( wp_scripts(), 'amp-paired-browsing-client', $handle ) ) {
+			$tag = preg_replace( '/(?<=<script)(?=\s|>)/i', ' ' . AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, $tag );
+		}
+
+		return $tag;
+	}


FYI: This should be able to be largely eliminated if we implement this: google/site-kit-wp#505 (comment)

westonruter

A big effort and a great feature.

westonruter · 2019-11-30T23:59:56Z

includes/class-amp-theme-support.php

+		wp_enqueue_script(
+			'amp-paired-browsing-client',
+			amp_get_asset_url( '/js/amp-paired-browsing-client.js' ),
+			$dependencies,
+			$version,
+			true
+		);
+
+		// Force dev mode to be enabled. This ensures that the enqueued script and its dependencies
+		// will be present when the admin bar is not showing.
+		add_filter( 'amp_dev_mode_enabled', '__return_true' );


@pierlon I just realized that this makes every page invalid AMP, because every page is forced to be in dev mode and the custom script is enqueued. Two options I see:

Short-circuit this function if an user is not logged-in (or rather, if amp_is_dev_mode() returns false). Since search crawlers will never be logged-in, they will never then see an invalid AMP page. This would also require entire paired browsing app should wp_die() if the user is not logged in.

Go back to only including this JS if the admin bar is showing. Accessing paired browsing here would still need to include the wp_die() if ! amp_is_dev_mode().

westonruter · 2019-12-01T12:26:02Z

I started experimenting with portals in #3864.

googlebot added the cla: yes Signed the Google CLA label Oct 28, 2019

pierlon requested review from westonruter, schlessera and kienstra October 28, 2019 21:59

swissspidy reviewed Oct 28, 2019

View reviewed changes

assets/src/admin/paired-browsing/app.js Outdated Show resolved Hide resolved

swissspidy reviewed Oct 28, 2019

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

pierlon force-pushed the enhancement/3365-paired-browsing branch from 9d49114 to 959d21b Compare October 28, 2019 23:22

westonruter changed the title ~~Add paired browsing interface for Transitional mode~~ Add paired browsing interface Oct 29, 2019

pierlon force-pushed the enhancement/3365-paired-browsing branch from 050e401 to 80f0cee Compare October 29, 2019 01:21

swissspidy reviewed Oct 29, 2019

View reviewed changes

assets/src/admin/paired-browsing/app.js Show resolved Hide resolved

swissspidy reviewed Oct 29, 2019

View reviewed changes

webpack.config.js Show resolved Hide resolved

swissspidy reviewed Oct 29, 2019

View reviewed changes

includes/class-amp-theme-support.php Show resolved Hide resolved

swissspidy reviewed Oct 29, 2019

View reviewed changes

includes/class-amp-theme-support.php Show resolved Hide resolved

swissspidy reviewed Oct 31, 2019

View reviewed changes

templates/amp-paired-browsing.php Outdated Show resolved Hide resolved

swissspidy reviewed Oct 31, 2019

View reviewed changes

assets/src/admin/paired-browsing/app.js Outdated Show resolved Hide resolved

Navigate parent window to disconnected client upon exiting

b684aa2

westonruter reviewed Nov 30, 2019

View reviewed changes

assets/src/admin/paired-browsing/app.js Outdated Show resolved Hide resolved

Use gray background color for non-AMP half of screen

671e6c0

westonruter added 2 commits November 29, 2019 21:21

Let Non-AMP and AMP headings be links

f56ab39

Eliminate (redundant) Paired Browsing heading and Exit link

02aff70

Use 'AMP Paired Browsing:' as prefix for document title

72f761d

westonruter added 5 commits November 30, 2019 15:23

Omit paired browsing admin bar item when invalid markup present

2097e41

Remove all validation query vars when entering paired browsing

42a4c92

Add helper method to get paired browsing URL and use in links up front

7824d05

Remove 'start' from paired browsing; use link instead of button

53c78f4

westonruter reviewed Nov 30, 2019

View reviewed changes

westonruter added 2 commits November 30, 2019 17:48

Remove extraneous method in favor of (temporary) closure

0b2caa8

Add robots noindex/nofollow meta tag in paired browsing app

a13bddd

westonruter approved these changes Nov 30, 2019

View reviewed changes

westonruter merged commit 9bde137 into develop Nov 30, 2019

westonruter deleted the enhancement/3365-paired-browsing branch November 30, 2019 09:30

westonruter reviewed Nov 30, 2019

View reviewed changes

This was referenced Dec 1, 2019

Restrict paired browsing to when dev mode is enabled #3863

Merged

Try: Use portals instead of iframes in paired browsing, when available #3864

Closed

westonruter added this to the v1.5 milestone Jan 16, 2020

westonruter added the Changelogged label Mar 23, 2020

westonruter mentioned this pull request Dec 30, 2020

Add customization of Paired AMP URLs #5558

Merged

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add paired browsing interface #3656

Add paired browsing interface #3656

pierlon commented Oct 28, 2019 •

edited by westonruter

Loading

pierlon commented Oct 28, 2019

schlessera commented Oct 28, 2019

westonruter commented Oct 29, 2019

westonruter commented Oct 29, 2019 •

edited

Loading

pierlon commented Oct 29, 2019

westonruter commented Oct 29, 2019

pierlon commented Oct 29, 2019

pierlon commented Oct 29, 2019

pierlon commented Oct 29, 2019

swissspidy Oct 29, 2019

swissspidy Oct 29, 2019

swissspidy commented Oct 29, 2019

pierlon commented Oct 30, 2019

pierlon commented Oct 30, 2019

pierlon commented Oct 31, 2019

westonruter commented Nov 30, 2019

westonruter commented Nov 30, 2019

westonruter commented Nov 30, 2019

westonruter commented Nov 30, 2019

pierlon commented Nov 30, 2019

westonruter commented Nov 30, 2019

westonruter Nov 30, 2019

westonruter left a comment

westonruter Nov 30, 2019

westonruter Dec 1, 2019

westonruter commented Dec 1, 2019


		app.registerClientWindow( window );

		document.addEventListener( 'DOMContentLoaded', () => {

	<span>Paired Browsing</span>
	<span><?php esc_html_e( 'Paired Browsing', 'amp' ); ?></span>

Add paired browsing interface #3656

Add paired browsing interface #3656

Conversation

pierlon commented Oct 28, 2019 • edited by westonruter Loading

Summary

Todo

Checklist

pierlon commented Oct 28, 2019

schlessera commented Oct 28, 2019

westonruter commented Oct 29, 2019

westonruter commented Oct 29, 2019 • edited Loading

pierlon commented Oct 29, 2019

westonruter commented Oct 29, 2019

pierlon commented Oct 29, 2019

pierlon commented Oct 29, 2019

pierlon commented Oct 29, 2019

swissspidy Oct 29, 2019

Choose a reason for hiding this comment

swissspidy Oct 29, 2019

Choose a reason for hiding this comment

swissspidy commented Oct 29, 2019

pierlon commented Oct 30, 2019

pierlon commented Oct 30, 2019

pierlon commented Oct 31, 2019

westonruter commented Nov 30, 2019

westonruter commented Nov 30, 2019

westonruter commented Nov 30, 2019

westonruter commented Nov 30, 2019

pierlon commented Nov 30, 2019

westonruter commented Nov 30, 2019

westonruter Nov 30, 2019

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

westonruter Nov 30, 2019

Choose a reason for hiding this comment

westonruter Dec 1, 2019

Choose a reason for hiding this comment

westonruter commented Dec 1, 2019

pierlon commented Oct 28, 2019 •

edited by westonruter

Loading

westonruter commented Oct 29, 2019 •

edited

Loading